Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(digest): Digest wrappers #102

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

hahnandrew
Copy link

@hahnandrew hahnandrew commented Sep 30, 2024

Adds rust wrappers for module APIs ValkeyModule_DigestAddStringBuffer, ValkeyModule_DigestAddLongLong, ValkeyModule_DigestEndSequence, ValkeyModule_GetKeyNameFromDigest, ValkeyModule_GetDbIdFromDigest

Adds callback for digest and integration test for alloc module

Closes #97

@hahnandrew hahnandrew force-pushed the feat/module-digest branch 3 times, most recently from 9997cb4 to b1409c8 Compare October 2, 2024 02:26

pub fn db_id(&self) -> c_int {
unsafe { (*self.dig).dbid }
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can implement wrapper functions for the following operations:

ValkeyModule_DigestAddStringBuffer
ValkeyModule_DigestAddLongLong
ValkeyModule_DigestEndSequence
ValkeyModule_GetKeyNameFromDigest
ValkeyModule_GetDbIdFromDigest

For example, we can add a function named add_long_long.
This function will look something like:

pub fn add_long_long(&mut self, ll: i64) {
    unsafe {
        // Make a call to raw::RedisModule_DigestAddLongLong API and mutatively pass "self.dig" and "ll" c_longlong type
    }
}

Copy link
Author

@hahnandrew hahnandrew Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback Karthik - updated

ValkeyModule_DigestAddStringBuffer
ValkeyModule_DigestAddLongLong
ValkeyModule_DigestEndSequence
ValkeyModule_GetKeyNameFromDigest
ValkeyModule_GetDbIdFromDigest

Signed-off-by: Andrew Hahn <[email protected]>
src/digest.rs Outdated
/// # Panics
///
/// Will panic if `RedisModule_GetKeyNameFromDigest` is missing in redismodule.h
pub fn get_key_name(&self) -> *const raw::RedisModuleString {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To help the caller functions not operate on raw pointers, can we update this to return a ValkeyString type?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, updated

pub dig: *mut raw::RedisModuleDigest,
}

impl Digest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: For all the unsafe functions where we are unpacking the Module API into an optional function, we can use a simpler syntax with expect. Example:

unsafe {
    raw::RedisModule_GetKeyNameFromDigest
        .expect("RedisModule_GetKeyNameFromDigest is not available.")(self.dig)
}

Copy link
Author

@hahnandrew hahnandrew Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great feedback - updated

@@ -11,6 +11,7 @@ mod redismodule;
pub mod redisraw;
pub mod redisvalue;
pub mod stream;
pub mod digest;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. It would be good to test out this change. We can do this by using the Digest code added and updating this example module (data_type.rs) to implement a callback for the digest function.
https://github.com/valkey-io/valkeymodule-rs/blob/main/examples/data_type.rs

Here is a small example in C where we do the same: https://github.com/valkey-io/valkey/blob/b0f23df16522e91a769c75646166045ae70e8d4e/src/modules/hellotype.c#L296

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add a test, we can add a new function for the "alloc" module here: https://github.com/valkey-io/valkeymodule-rs/blob/main/tests/integration.rs

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, added

@hahnandrew hahnandrew force-pushed the feat/module-digest branch 4 times, most recently from 02997a1 to 8d96c9b Compare October 16, 2024 06:24
@hahnandrew hahnandrew marked this pull request as ready for review October 25, 2024 04:36
@@ -736,3 +736,53 @@ fn test_expire() -> Result<()> {

Ok(())
}

#[test]
fn test_alloc_data_type() -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding a test for this Module.

To test the new functionality (digest), we can use the DEBUG command using redis::cmd("DEBUG") on the module data type key and validate that it returns a value without raising any error. This will validate your new callback is being invoked

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, updated

#[test]
fn test_alloc_data_type() -> Result<()> {
let port: u16 = 6503;
let _guards = vec![start_valkey_server_with_module("data_type", port)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the name of the Module alloc or data_type?

Copy link
Author

@hahnandrew hahnandrew Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alloc, but start_valkey_server_with_module takes in the file name, which is data_type here - I'll update it to be clear, thanks for the feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification. That makes sense

@@ -736,3 +736,53 @@ fn test_expire() -> Result<()> {

Ok(())
}

#[test]
Copy link
Member

@KarthikSubbarao KarthikSubbarao Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: does this test (test_alloc_data_type ) get executed when you run the cargo test command? Just want to validate that it runs / passes locally for you

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Member

@KarthikSubbarao KarthikSubbarao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Andrew. This looks good to me. Approved

#[test]
fn test_alloc_data_type() -> Result<()> {
let port: u16 = 6503;
let _guards = vec![start_valkey_server_with_module("data_type", port)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification. That makes sense


// Test DEBUG digest command to verify digest callback with current key
let res: String = redis::cmd("DEBUG")
.arg(&["digest"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the key name "test_key"? If so, we can update this arg to be "test_key"

Copy link
Author

@hahnandrew hahnandrew Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, but digest doesn't have any args and uses what's set through alloc.set for its hash input. The syntax looks confusing, but .args is used for both subcommands or as arguments to the first command

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding both the tests. This helps add coverage on this new functionality

@hahnandrew hahnandrew force-pushed the feat/module-digest branch 2 times, most recently from cdd7fb7 to 6fd0483 Compare October 30, 2024 18:26
Signed-off-by: Andrew Hahn <[email protected]>
@dmitrypol
Copy link
Collaborator

hi @hahnandrew . Thank for submitting the PR. I am sorry it took me so long to get to it. Left a few comments.

@dmitrypol
Copy link
Collaborator

dmitrypol commented Nov 10, 2024

when I run test.sh (after adding missing }) I get

error[E0308]: mismatched types
   --> tests/integration.rs:791:20
    |
791 | fn test_debug() -> Result<()> {
    |    ----------      ^^^^^^^^^^ expected `Result<(), Error>`, found `()`

You need to add Ok(()) at the end

@dmitrypol
Copy link
Collaborator

also could you please run cargo fmt to format code nicely, I know it's a nitpick

let mut dig = Digest::new(md);
let val = &*(value.cast::<MyType>());
dig.add_string_buffer(&val.data.as_bytes());
dig.end_sequence();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we exercise add_string_buffer and end_sequence via Integration Tests. How can we add tests to also test get_key_name, get_db_id and add_long_long?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for calling this out. I'm thinking that we can follow core and integration test get_key_name, get_db_id and add_long_long

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KarthikSubbarao - your thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it makes sense to test all the new functionality that being added, so I agree with your comment, @dmitrypol .

@hahnandrew That makes sense. We can follow the core and have the Digest callback of the example data type use all the new wrapper Digest APIs added.

Here is how the core tests the Module APIs (using the core example module).

Core Unit Tests written in TCL: https://github.com/valkey-io/valkey/blob/2df56d87c0ebe802f38e8922bb2ea1e4ca9cfa76/tests/unit/moduleapi/datatype2.tcl#L183

Core Module: https://github.com/valkey-io/valkey/blob/b0f23df16522e91a769c75646166045ae70e8d4e/tests/modules/datatype2.c#L614

For the get_db_id, notice how they use SELECT to switch data bases. We can do the same and validate that the APIs work as expected

(temp): scaffold for get_key_name, get_db_id, and add_long_long tests

Signed-off-by: Andrew Hahn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for implementing digest Module type callback using existing Module APIs
3 participants